Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the default input for showProfile API #2343

Closed
wants to merge 4 commits into from
Closed

Conversation

maglims
Copy link
Contributor

@maglims maglims commented May 29, 2024

According to our official documentation, the available TriggerType for profile api are these: TriggerType = "MouseHover" | "Press" | "AppRequest"
https://learn.microsoft.com/en-us/javascript/api/@microsoft/teams-js/profile.triggertype?view=msteams-client-js-latest

Updating our default input to use the correct one "MouseHover" instead of "MouseClick". This will clear some confusions for devs who are using the default input while doing manual testing. Also, it will not work with "MouseClick" input(throwing an exception back to app) because it doesn't match the enum defined in hub sdk, which is "MouseHover".

Description

See above

If this Pull Request should close/resolve any issues when merged, use the special syntax for that here.

Main changes in the PR:

  1. Update from "MouseClick" to "MouseHover" for the default input.

Validation

Validation performed:

  1. <Step 1>
  2. <Step 2>

Unit Tests added:

Unit tests are required for all changes. If no unit tests were added as part of this change, please explain why they aren't necessary.

<Yes/No>

End-to-end tests added:

<Yes/No>

Additional Requirements

Change file added:

Ensure the change file meets the formatting requirements.

<Yes/No>

Related PRs:

Remove this section if n/a

Next/remaining steps:

List the next or remaining steps in implementing the overall feature in subsequent PRs (or is the feature 100% complete after this?).

Remove this section if n/a

  • Item 1
  • Item 2

Screenshots:

Remove this section if n/a

Before After
< image1 > < image2 >

@maglims maglims requested a review from a team as a code owner May 29, 2024 20:28
@@ -23,9 +23,9 @@
"width": 0,
"height": 0
},
"triggerType": "MouseClick"
"triggerType": "MouseHover"
Copy link
Contributor

@KangxuanYe KangxuanYe May 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am so confused how could it work for web that you didn't make any change in Host SDK side but simply replace MouseClick to MouseHover? It is not an input so I assume it should have a failure here. I am not familiar with this part of validation and I would like to know more about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe due to this is the default input for the test app... for example, in iOS, we also don't use this default value for testing. I'd image the default values are mostly used by devs when they are doing the manual testing (so they can just use default and don't have to manually find the input). Does it sound reasonable?

Copy link
Contributor

@TrevorJoelHarris TrevorJoelHarris Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change would be needed in the host-sdk because the host-sdk is just taking this entire payload it receives as part of the call and displaying the whole thing it in an alert. In this case, this test is only used in the web, and it's passing up MouseHover as the trigger type, and so the test is expecting that to show up in an alert.

The default value in ProfileAPIs.tsx is only added if a user presses the "Default" button in the test app, it's intended to make manual testing using the test app more straightforward, it can be tricky to figure out what format each call wants added to the text box without it. Making a change there should have no effect on the E2E tests.

@TrevorJoelHarris TrevorJoelHarris self-assigned this Jul 1, 2024
@TrevorJoelHarris
Copy link
Contributor

Spoke with @maglims and was told this was no longer needed because the test issues were addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants